Skip to content

Conversation

@pawosm-arm
Copy link
Contributor

The command line reality is this:

$ clang -c prog.c -fveclib=accelerate
error: invalid value 'accelerate' in '-fveclib=accelerate'

$ clang -c prog.c -fveclib=Accelerate
prog.c:1:2: warning: This is only a test [-W#warnings]
1 | #warning This is only a test
| ^
1 warning generated.

$ clang -c prog.c -fveclib=libmvec
prog.c:1:2: warning: This is only a test [-W#warnings]
1 | #warning This is only a test
| ^
1 warning generated.

$ clang -c prog.c -fveclib=LIBMVEC
error: invalid value 'LIBMVEC' in '-fveclib=LIBMVEC'

$ clang -c prog.c -fveclib=massv
error: invalid value 'massv' in '-fveclib=massv'

$ clang -c prog.c -fveclib=MASSV
prog.c:1:2: warning: This is only a test [-W#warnings]
1 | #warning This is only a test
| ^
1 warning generated.

$ clang -c prog.c -fveclib=sleef
error: invalid value 'sleef' in '-fveclib=sleef'

$ clang -c prog.c -fveclib=sleefgnuabi
error: invalid value 'sleefgnuabi' in '-fveclib=sleefgnuabi'

$ clang -c prog.c -fveclib=SLEEF
prog.c:1:2: warning: This is only a test [-W#warnings]
1 | #warning This is only a test
| ^
1 warning generated.

$ clang -c prog.c -fveclib=darwin_libsystem_m
error: invalid value 'darwin' in '-fveclib=darwin_libsystem_m'

$ clang -c prog.c -fveclib=Darwin_libsystem_m
prog.c:1:2: warning: This is only a test [-W#warnings]
1 | #warning This is only a test
| ^
1 warning generated.

$ clang -c prog.c -fveclib=armpl
error: invalid value 'armpl' in '-fveclib=armpl'

$ clang -c prog.c -fveclib=ARMPL
error: invalid value 'ARMPL' in '-fveclib=ARMPL'

$ clang -c prog.c -fveclib=ArmPL
prog.c:1:2: warning: This is only a test [-W#warnings]
1 | #warning This is only a test
| ^
1 warning generated.

$ clang -c prog.c -fveclib=amdlibm
error: invalid value 'amdlibm' in '-fveclib=amdlibm'

$ clang -c prog.c -fveclib=AMDLIBM
clang: error: unsupported option 'AMDLIBM' for target 'aarch64'

…atch with the reality

The command line reality is this:

$ clang -c prog.c -fveclib=accelerate
error: invalid value 'accelerate' in '-fveclib=accelerate'

$ clang -c prog.c -fveclib=Accelerate
prog.c:1:2: warning: This is only a test [-W#warnings]
    1 | #warning This is only a test
      |  ^
1 warning generated.

$ clang -c prog.c -fveclib=libmvec
prog.c:1:2: warning: This is only a test [-W#warnings]
    1 | #warning This is only a test
      |  ^
1 warning generated.

$ clang -c prog.c -fveclib=LIBMVEC
error: invalid value 'LIBMVEC' in '-fveclib=LIBMVEC'

$ clang -c prog.c -fveclib=massv
error: invalid value 'massv' in '-fveclib=massv'

$ clang -c prog.c -fveclib=MASSV
prog.c:1:2: warning: This is only a test [-W#warnings]
    1 | #warning This is only a test
      |  ^
1 warning generated.

$ clang -c prog.c -fveclib=sleef
error: invalid value 'sleef' in '-fveclib=sleef'

$ clang -c prog.c -fveclib=sleefgnuabi
error: invalid value 'sleefgnuabi' in '-fveclib=sleefgnuabi'

$ clang -c prog.c -fveclib=SLEEF
prog.c:1:2: warning: This is only a test [-W#warnings]
    1 | #warning This is only a test
      |  ^
1 warning generated.

$ clang -c prog.c -fveclib=darwin_libsystem_m
error: invalid value 'darwin' in '-fveclib=darwin_libsystem_m'

$ clang -c prog.c -fveclib=Darwin_libsystem_m
prog.c:1:2: warning: This is only a test [-W#warnings]
    1 | #warning This is only a test
      |  ^
1 warning generated.

$ clang -c prog.c -fveclib=armpl
error: invalid value 'armpl' in '-fveclib=armpl'

$ clang -c prog.c -fveclib=ARMPL
error: invalid value 'ARMPL' in '-fveclib=ARMPL'

$ clang -c prog.c -fveclib=ArmPL
prog.c:1:2: warning: This is only a test [-W#warnings]
    1 | #warning This is only a test
      |  ^
1 warning generated.

$ clang -c prog.c -fveclib=amdlibm
error: invalid value 'amdlibm' in '-fveclib=amdlibm'

$ clang -c prog.c -fveclib=AMDLIBM
clang: error: unsupported option 'AMDLIBM' for target 'aarch64'
Copy link
Contributor

@arsenm arsenm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The random capitalizations are annoying, and don't even match the backend flag's spelling. We probably should accept the lowercase (it's weird that these weren't all lowercase to start with)

@pawosm-arm
Copy link
Contributor Author

pawosm-arm commented Nov 16, 2025

We probably should accept the lowercase

I suspect a commit that would permit lowercase spelling of those (I assume along with the current widely-used spelling) would be impossible to get through considering the places it would touch (Clang.cpp, MSVC.cpp, Flang.cpp, a number of places in CommonArgs.cpp, and of course Options.td). Adding std::tolower on each relevant string comparisons in the C++ files would look uncomfortable enough, but adding lowercase counterparts to each available veclib option in the Options.td file would look even more uncomfortable (and concerning to the most of the reviewers). IMO this is not the right thing to do. The milk is spilled, the current spellings of these options were accepted by reviewers, the users have made uses of this spelling, now the only thing we can do is to make the documentation reflect that.

Changing the spelling to all-lowercase (to match with the documentation) is also no-go area: people wrote Makefiles and other codebase artifacts using the current spelling. The veclib flags are mostly used by HPC apps, they tend to be large, people will be cursing if we do a change which will force them to make changes in their chunky build scripts.

@arsenm
Copy link
Contributor

arsenm commented Nov 16, 2025

Adding std::tolower on each relevant string comparisons in the C++ files would look uncomfortable enough,

We have StringRef::equals_insensitive

but adding lowercase counterparts to each available veclib option in the Options.td file would look even more uncomfortable (and concerning to the most of the reviewers).

I don't see how this is burdensome

Copy link
Contributor

@kiranchandramohan kiranchandramohan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updating the documentation to show the correct values that are accepted makes sense to me. Any further work should be taken up in a separate patch by interested parties after writing an RFC in discourse.

LGTM. Please wait for a reviewer from Clang Driver/LLVM vectorizer.

Copy link
Contributor

@fhahn fhahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updating the documented names to match the flags is a good step. As @arsenm suggested, it would be more user friendly to also accept the lower-case variants.

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, agreed with changing the driver to accept varying cases though.

@pawosm-arm
Copy link
Contributor Author

pawosm-arm commented Nov 17, 2025

We have StringRef::equals_insensitive

We do. But using == operator for a string comparison always looks better. I won't be arguing on this though, as I'm seeing it (the StringRef::equals_insensitive) used frequently throughout the frontend driver code. But then, what would you do about this?:

  // List of veclibs which when used with -fveclib imply -fno-math-errno.
  constexpr std::array VecLibImpliesNoMathErrno{llvm::StringLiteral("ArmPL"),
                                                llvm::StringLiteral("SLEEF")};

...

    case options::OPT_fveclib:
      VecLibArg = A;
      NoMathErrnoWasImpliedByVecLib =
          llvm::is_contained(VecLibImpliesNoMathErrno, A->getValue());
      if (NoMathErrnoWasImpliedByVecLib)
        MathErrno = false;

There's no llvm::is_contained_case_insensitive available, so what would be a good LLVM idiom for handling this?

NB: this is one of the places I have spotted, I wonder how many other places will be affected non-obvious way if we allow case insensitive spelling of the names which were used in their current forms for the last few years...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants